Skip to content

Sub-PR of 846: Modification of Updater and tests as part of #660 #868

Closed
adityasaky wants to merge 16 commits intotheupdateframework:660_respect_delegation_graphfrom
adityasaky:660_updater
Closed

Sub-PR of 846: Modification of Updater and tests as part of #660 #868
adityasaky wants to merge 16 commits intotheupdateframework:660_respect_delegation_graphfrom
adityasaky:660_updater

Conversation

@adityasaky
Copy link
Collaborator

@adityasaky adityasaky commented May 1, 2019

Part of the #660 effort, a sub-PR of #846. This is part of step 3 of the plan outlined here: #846 (comment)

adityasaky and others added 16 commits May 1, 2019 11:49
@adityasaky @awwad
Update format of storing delegated roles' metadata
b4ee6dc
@adityasaky @awwad
Remove pdb statements
cb3d132
@adityasaky @awwad
Update schema checks to match format changes introduced
3a0090b
@adityasaky @awwad
Update method names following roledb changes
167db5c
@adityasaky @awwad
Add initial arguments for keyids and threshold
8eeac96
@adityasaky @awwad
Pass threshold and keyids down preorder walk stack
aa94e1b
@adityasaky @awwad
Update expected values and exceptions for _import_delegations
60232b8
@adityasaky @awwad
Fix tuf.roledb reference
f65bd95
@adityasaky @awwad
Add expectedFailure to test for now
9b6fcb5
@adityasaky @awwad
Add return values for keyids, threshold, update roledb method calls
7d85046
@adityasaky @awwad
Update roleinfo keys to reflect changes made in roledb
0a44faf
@adityasaky @awwad
Pass only rolename to sig.verify
aba9ec6
@adityasaky @awwad
Add TODO for assertion fix
f75368b
@adityasaky @awwad
Add manual test for get_one_valid_targetinfo for non top-level targets
b3fcac1
@adityasaky @awwad
Add TODO to remove redundant line
7a72d82
@awwad
Testing: Reset spec version if a failed test changed it
c0c9fc2 
A test in test_updater modifies tuf.SPECIFICATION_VERSION
temporarily.  If it fails, it doesn't get to reset the value.

This commit ensures that tuf.SPECIFICATION_VERSION is an
expected value at the start of each test -- specifically, the
value it had when this test module was first imported.

I thought about using a try/except/else structure, but the span
of the test was too long and I didn't want to nest try/except.

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
@awwad
Copy link
Contributor

awwad commented May 2, 2019

@adityasaky
Of the 14 test failures:

  • One or two were handled when I added a commit just now that made sure that tuf.SPECIFICATION_VERSION is set correctly at the start of each test, so that if the test that modifies it fails before it can switch it back to the original value, things don't break in other tests.
  • Some failures are related to Updater.targets_of_role and Updater.all_targets. I'll handle those functions (and their tests), either in this sub-PR or in 846 itself.
  • Many of the tests use repository_tool to do test set up. repository_tool fixes will not be included in this sub-PR. While we could alter these tests to use static metadata or otherwise avoid repository_tool use, this is probably more effort than it's worth, so I'll eyeball the underlying functionality to make sure nothing obvious is missing. Then, we can see what the test results are after the repository_tool sub-PR.
  • It looks like some of them are related to Repository.load_repository not loading all roles the way it now should. That we can leave for the next sub-PR, too. (test_5_targets_of_role, test_8_remove_obsolete_targets)

@awwad
Copy link
Contributor

awwad commented May 2, 2019 •
edited
Loading

Now that roledb has been cleansed with fire, it looks more and more like updater shouldn't use roledb at all: it already has its own store of role metadata (self.metadata), and I don't think it's helping anyone to try to keep that synched. Some things check self.metadata, and some check roledb.... Further, Updater instances keep two versions of metadata: the currently trusted as well as the previously trusted, so roledb isn't adequate for its purposes anyway. (It could even cause weird issues if you're running a few different things in one process -- a client and repo tool, or two clients, say.) I have to think through that just a little bit more. It's probably still reasonable for updater to use keydb.... I'll figure this out in the parent PR after this is merged. I'm going to clean up some tests and updater code here, though.

It also looks like class Updater should swallow class MultiRepoUpdater, but that's definitely issue for another day.

@awwad awwad mentioned this pull request Jul 23, 2019
Closed
@joshuagl
Copy link
Member

joshuagl commented Sep 10, 2020

Due to imminent refactor efforts this code is unlikely to merge. Thanks for your efforts in the PR!

@joshuagl joshuagl closed this Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adityasaky @awwad @joshuagl